Implement the RRM express setup dashboard entry point.#13107
Open
JakePT wants to merge 25 commits into
Open
Conversation
🎭 Playwright reports for a67fc15: 📚 Storybook for a67fc15: 📦 Build files for a67fc15:
|
JakePT
marked this pull request as ready for review
July 14, 2026 07:10
Collaborator
Author
|
Failing E2E tests are the usual suspects and VRT seems unrelated and passes locally. |
|
Size Change: +12.5 kB (+0.34%) Total Size: 3.73 MB 📦 View Changed
ℹ️ View Unchanged
|
JakePT
commented
Jul 14, 2026
JakePT
commented
Jul 14, 2026
|
|
||
| @media (min-width: $width-tablet + 1 + px) { | ||
| p.googlesitekit-banner__title { | ||
| @include googlesitekit-typography(headline, small); |
Collaborator
Author
There was a problem hiding this comment.
I thought a mixin like this would be useful for re-using typography styles on elements that aren't using the Typography component, like the banner heading, and need to switch between styles at different breakpoints.
Collaborator
There was a problem hiding this comment.
This looks clean, fantastic work!
Applying this mixin means overriding more styles than we need to, because the Banner component by default matches most of the Figma designs, but it does look much cleaner, easier to read, and more referable to the Figma designs.
JakePT
commented
Jul 15, 2026
nfmohit
requested changes
Jul 19, 2026
|
|
||
| @media (min-width: $width-tablet + 1 + px) { | ||
| p.googlesitekit-banner__title { | ||
| @include googlesitekit-typography(headline, small); |
Collaborator
There was a problem hiding this comment.
This looks clean, fantastic work!
Applying this mixin means overriding more styles than we need to, because the Banner component by default matches most of the Figma designs, but it does look much cleaner, easier to read, and more referable to the Figma designs.
Co-authored-by: Nahid F Mohit <nahid.mohit@10up.com>
nfmohit
requested changes
Jul 22, 2026
Co-authored-by: Nahid F Mohit <nahid.mohit@10up.com>
Co-authored-by: Nahid F Mohit <nahid.mohit@10up.com>
Co-authored-by: Nahid F Mohit <nahid.mohit@10up.com>
Co-authored-by: Nahid F Mohit <nahid.mohit@10up.com>
Collaborator
Author
|
@nfmohit GitHub's threads are becoming unwieldy but I think I've responded to or fixed everything. |
nfmohit
requested changes
Jul 24, 2026
JakePT
commented
Jul 24, 2026
| <Widget noPadding> | ||
| <Banner | ||
| className="googlesitekit-rrm-setup-cta-banner" | ||
| className="googlesitekit-rrm-setup-cta-banner-widget" |
Collaborator
Author
There was a problem hiding this comment.
@nfmohit Tiny nitpick, but I'd interpret googlesitekit-rrm-setup-cta-banner-widget as representing the ReaderRevenueManagerSetupCTABannerWidget, but since this class is styling the <Banner> inside the <Widget> I thought googlesitekit-rrm-setup-cta-banner would be more appropriate/accurate. It's styles for a banner component rather than widget component, if that makes sense.
Collaborator
There was a problem hiding this comment.
Good point. Let's update the class name to googlesitekit-rrm-express-setup-widget-banner. We should ideally rename the component (and stylesheet) too to something like RRMExpressSetupBannerWidget, to avoid its resemblance to the existing ReaderRevenueManagerSetupCTABanner. WDYT?
Collaborator
Author
There was a problem hiding this comment.
@nfmohit Well there's a bit of inconsistency with how components are named because some use ReaderRevenueManager, some use RRM and some are un-prefixed, but looking at examples in other modules I'd be tempted to do this (with or without RRM prefixes):
assets/js/modules/reader-revenue-manager/components/dashboard/ExpressSetupBannerWidgetExpressSetupBanner.tsxExpressSetupBanner<Banner className="googlesitekit-rrm-express-setup-banner" />
index.tsxExpressSetupBannerWidget<Widget><ExpressSetupBanner>
index.test.tsindex.stories.ts
ExpressSetupBanner would own the copy and presentation while ExpressSetupBannerWidget would control visibility and pass down the button handlers.
WDYT?
nfmohit
requested changes
Jul 24, 2026
Co-authored-by: Nahid F Mohit <nahid.mohit@10up.com>
…ReaderRevenueManagerSetupCTABannerWidget.tsx Co-authored-by: Nahid F Mohit <nahid.mohit@10up.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist